ci: report disguised file content to code scanning - #4056
Conversation
Nothing in CI inspects the content of files a change adds. A payload named like an inert asset is skipped by any scanner that selects files by extension, which is how an implant force-pushed onto the head branch of an open pull request went unexamined. Classify every changed file by content with magika and report any file whose bytes are code or an executable while its extension declares a binary asset. Findings are emitted as SARIF and uploaded to code scanning, so severity, per-finding dismissal and merge blocking are configured in a ruleset rather than hand-maintained here. The script therefore exits 0 on a finding, and reserves a non-zero exit for being unable to scan at all. Cannot-scan is treated as failure throughout, because each case otherwise looks identical to a clean run: a base or head commit missing from the clone makes the diff empty, magika exits non-zero for a file it could not read, and a path holding a byte above 0x80 is quoted by git and drops out of the file list unless the diff is read with -z. Fork pull requests cap security-events at read, so the upload cannot run for them; those fail the job directly and annotate the offending lines instead.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
There was a problem hiding this comment.
🟡 Not ready to approve
The scan script can exit with the wrong status (and potentially leave stale/partial SARIF) on output write failures, undermining the “cannot-scan must fail clearly” contract.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds a new CI security workflow that classifies the content of every changed file using Magika and reports “executable/code content under inert-asset extensions” as SARIF to GitHub code scanning, so disguised payloads can be surfaced (and optionally blocked) via branch rulesets rather than bespoke status-check gating.
Changes:
- Introduces a
Security Scanworkflow that runs on bothpushandpull_request, uploads SARIF on pushes, and fails fork PRs directly on findings. - Adds a pinned Magika CLI fetch/install script for CI plus a local Nix derivation/devshell wiring for parity.
- Implements the detector (Magika JSONL → SARIF) and a throwaway-repo test suite covering key failure modes and bypass techniques.
File summaries
| File | Description |
|---|---|
nix/magika.nix |
Adds a pinned, checksummed Magika CLI derivation (prebuilt upstream binaries) for local/devshell use. |
flake.nix |
Exposes magika in the dev shell so the scan script/tests can be run locally. |
.github/workflows/security-scan.yml |
New workflow to run the scan on pushes/PRs and report via SARIF/code scanning. |
.github/security-scan/README.md |
Documents rationale, local usage, merge blocking model, and version pinning. |
.github/scripts/tests/test-scan-changed-files.sh |
Adds regression tests for bypasses and “cannot scan” failure cases. |
.github/scripts/scan-changed-files.sh |
Orchestrates git diff enumeration, Magika invocation, SARIF generation, and auto-exec path warnings. |
.github/scripts/find-type-mismatches.py |
Converts Magika JSONL output into a SARIF run/results for code scanning. |
.github/scripts/fetch-scanners.sh |
CI-only downloader for the pinned Magika CLI release artifact with checksum verification. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| write_sarif() { | ||
| python3 "$MISMATCH_SCRIPT" > "$workdir/sarif" || die "Could not write SARIF." | ||
| mv "$workdir/sarif" "$SARIF_OUT" | ||
| } |
| "$dest/magika" --version | grep -q "$MAGIKA_VERSION" \ | ||
| || { echo "magika is not version $MAGIKA_VERSION" >&2; exit 1; } |
Pull request overviewAdds a content-based scan of every changed file: Changes:
Reviewed changesPer-file summary
FindingsBlocking (must fix before merge):
Non-blocking (nits, follow-ups, suggestions):
|
magika ships a Python API, so classifying files no longer needs a CLI binary, JSONL parsing, or exit-code juggling across three shell scripts. Range resolution moves out of inline workflow bash into the scanner, and uv installs the pinned dependency from the script's own inline metadata, which removes the fetch script and the nix derivation along with their duplicated version. The workflow is now only `uses:` steps and single-command `run:` steps. A path with a newline no longer has to be refused: paths are passed as arguments rather than through a newline-delimited list.
permissions: {} at the top level, granted per job, matching astral-sh/uv and
apache/iceberg-rust. Records the conventions a second tool has to follow,
notably a SARIF category unique per tool, since code scanning keeps one
analysis per category.
Closes #4037. Supersedes #4038.
Classifies the content of every changed file with magika and reports any file whose bytes are code or an executable while its extension claims an inert asset. That is the implant we actually got: JavaScript named
fa-solid-400.woff2, padded with 1700 spaces to sit off-screen in the diff, force-pushed onto an open PR's head branch. Extension-filtering scanners never open such a file, which is why the disguise worked.One Python script, no shell, no bespoke gating:
uvinstalls magika from the script's own inline metadata, so the pinned version lives in one placegit diff -z, becausecore.quotePathquotes bytes above 0x80 and the quoted literal matches no file on disk, silently dropping a homoglyph-named payloadNeeds a repo admin
Without the first, this reports but blocks nothing.
code_scanningrule to thedefaultruleset onmain(repos/near/mpc/rulesets/2666105):tool: disguised-content,alerts_threshold: errors,security_alerts_threshold: none. It also blocks while analysis runs or if the tool is missing, so a job that never reports cannot look clean — which is why it is used instead of a required status check, since the skipped same-repo-PR run publishes the same check name and GitHub counts a skip as satisfied.require_last_push_approval: truein the same ruleset. Currentlyfalse: stale reviews are already dismissed on push and 2 approvals are required, but whoever pushed the new head can still supply one of them, which is the vector here.Deviation from #4037
The issue asks for YARA and magika; this ships magika only, and I think the issue is what should change. All 54 rules in GuardDog's pack carry
path_includeand none names.rs— coverage is Python (41 rules), JS/TS (41), Go (18), Ruby (10), shell (2). Running them over everything anyway is off-label, which is what forced 18 of 54 to advisory in #4038 plus a 54-line allowlist re-measured on every bump. For this payload the YARA hit was redundant: magika alone reports "content is javascript, name claims a font".Worth its own issue: this repo depends on 11 third-party Actions and checks none. GuardDog's
github-actionecosystem covers that natively, as an addition rather than a replacement.